Fix OpenPGP key-fingerprint panic under GODEBUG=fips140=only - #458
Conversation
47a44b0 to
6273fcb
Compare
|
@joaopapereira Please review the PR. |
joaopapereira
left a comment
joaopapereira
left a comment
There was a problem hiding this comment.
I do have some doubts about the implementation here and the removal of the FIPS check
| // Verifying a signature made with non-FIPS-approved | ||
| // algorithms panics under GODEBUG=fips140=only. | ||
| // Wrapping this call in WithoutEnforcement allows signature | ||
| // verification of legacy signed commits/tags without panicking. | ||
| fips140.WithoutEnforcement(func() { | ||
| _, err = openpgp.CheckArmoredDetachedSignature(publicKeys, target, sig) | ||
| }) |
There was a problem hiding this comment.
This change in itself is a big complicated, because here we are saying that we will no longer verify, if in a FIPS environment, the keys are good or not. Also not sure if we can consider this backwards compatible.
We could do something like parsing the signature packet, read its hash algorithm, and if it's non-FIPS (SHA-1/MD5) return a clear error; otherwise verify normally under enforcement?
Another option would be for us to try to recover the panic and just get an error out, not the greatest option but that would be an option as well
There was a problem hiding this comment.
I think erroring out isn't viable — it would break vendir sync for existing repos with older SHA-1/DSA-signed commits/tags, which is a real production concern for consumers.
So we added a pre-check (hash + public-key algorithm), but instead of erroring, we log a warning naming the non-approved algorithm and let verification proceed. Verification itself is unchanged — the signature/key is still checked and a bad match still fails; we only wrap the call in WithoutEnforcement so it doesn't panic on the non-approved primitive. Outside fips140=only this is a no-op, so it's backwards compatible.
There was a problem hiding this comment.
Thanks for explaining the production concern, that's legitimate and I don't want to make life harder for people with legacy signed history for no reason.
But I don't think warn-and-proceed should be the default here. The algorithm pre-check you already added is doing the hard part, so let's just use it to return an error instead of a log warning, and skip the CheckArmoredDetachedSignature call entirely when the algorithm isn't FIPS-approved. That means we don't need WithoutEnforcement around the actual verify call at all, only around the key-parsing/fingerprint step in armor.go, which I'm fine with since that's just a lookup id and not a trust decision.
That gets us a clean, actionable error under fips140=only instead of a panic or a silent pass that's easy to miss in CI. It also matches what someone turning that flag on is actually asking for. I'd rather fail loudly by default than have vendir quietly trust a SHA-1/DSA signature in a mode whose whole point is to disallow that.
For your production concern, let's add an explicit opt-out, something like --allow-legacy-signatures or a config field, so consumers who know they need to keep syncing repos with older signatures can do that on purpose instead of us deciding it for everyone.
Can you take a pass at reworking it this way? Should be a small change from what's already there, happy to re-review quickly once it's up.
There was a problem hiding this comment.
Done — reworked as suggested. The algorithm pre-check now returns an error by default and short-circuits before CheckArmoredDetachedSignature is ever called, so non-approved signatures never reach the verify call at all. WithoutEnforcement is gone from the default path entirely; it's now only used inside the new opt-in (when a caller explicitly sets it), since that's the one case where we do need to let a non-approved algorithm through to verify. Went with a config field (verification.allowLegacySignatures) over a CLI flag for the opt-out, since it composes better with how some of our consumers drive vendir purely through YAML config rather than extra CLI args.
There was a problem hiding this comment.
Note : kapp-controller (a primary consumer of vendir) invokes vendir as a subprocess, driving it entirely through a generated YAML configuration piped over stdin (vendir sync -f - --lock-file /dev/null) — it has no mechanism for passing additional CLI flags, which is why allowLegacySignatures is exposed as a configuration field here rather than a flag.
We also confirmed that kapp-controller does not currently set verification at all when building the git configuration it passes to vendir. Its AppFetchGit API type has no signature-verification concept today, so the code path this PR changes — including the previously-panicking behavior — is not reachable through any App or PackageInstall resource in existing kapp-controller deployments. Wiring up verification support (and, by extension, allowLegacySignatures) in kapp-controller would be net-new work, tracked separately from this PR.
1b7539c to
8f90ad1
Compare
8f90ad1 to
2517fc5
Compare
|
@joaopapereira Updated the PR as per you review comments. PTAL! |
|
@joaopapereira Please review. |
|
@aroradaman Please review the PR! |
2517fc5 to
5008f51
Compare
| return fmt.Errorf("Reading signature: %s", err) | ||
| } | ||
|
|
||
| var nonApproved []string |
There was a problem hiding this comment.
question: can we maintain a global list? Instead of computing everytime?
There was a problem hiding this comment.
Renamed to errs []error per your suggestion — it's the per-signature result of the algorithm check, not a static list, so it can't be hoisted to a global.
5008f51 to
5be8cb3
Compare
|
@aroradaman Addressed the review comments, PTAL! |
| return fmt.Errorf("Reading signature: %s", err) | ||
| } | ||
|
|
||
| var errs []error |
There was a problem hiding this comment.
nit, can we have []string here, as we are just collecting information.
There was a problem hiding this comment.
Updated to use []string instead of []error here.
5be8cb3 to
ded754f
Compare
Addressed all the requested changes.
adf5ad2 to
0e77089
Compare
Parsing public keys via openpgp.ReadArmoredKeyRing unconditionally computes an RFC 4880 V4 key fingerprint using SHA-1, which panics under GODEBUG=fips140=only before any signature is ever checked. The fingerprint is used only as a key identifier for later signature lookups; it plays no part in verifying or trusting a signature. Wrap the parsing call in crypto/fips140.WithoutEnforcement so key parsing succeeds under strict FIPS enforcement without weakening signature verification, which continues to enforce FIPS independently. Adds a unit test covering key parsing under GODEBUG=fips140=only. Signed-off-by: Sameer <sameer.khan@broadcom.com>
Verifying a signature made with a non-FIPS-approved hash or public-key algorithm (SHA-1, MD5, DSA) panics under GODEBUG=fips140=only. Under strict enforcement, reject such signatures outright by default, with an error naming the exact non-approved algorithm(s) and pointing at the new verification.allowLegacySignatures configuration field. Add verification.allowLegacySignatures to DirectoryContentsGitVerification for consumers who need to keep syncing repositories with legacy-signed commits/tags under FIPS enforcement: when set, a non-approved algorithm only logs a warning instead of failing, and the actual CheckArmoredDetachedSignature call is wrapped in crypto/fips140.WithoutEnforcement (only reached in this opt-in case, since it would otherwise panic on the same non-approved algorithm). Outside of strict enforcement, or when the signature already uses an approved algorithm, behavior is unchanged and WithoutEnforcement is never invoked. This is exposed as a configuration field rather than a CLI flag so that systems driving vendir purely through a YAML configuration document (rather than additional CLI arguments) can set it. Adds unit tests covering the hash/pubkey-algorithm pre-check and its reject/warn behavior. Signed-off-by: Sameer <sameer.khan@broadcom.com>
- Update Go version to 1.26.5 in go.mod - Bump carvel.dev/imgpkg dependency to v0.48.1 - Update golangci-lint to align with Go version - Run `go mod tidy` and `go mod vendor` to update go.sum and vendor/ Signed-off-by: Sameer <sameer.khan@broadcom.com>
0e77089 to
04a7788
Compare
|
I am still not ok with the fact that with this change if you decide you want fips for compliance you will no longer fail even if you are no compliant. Given that I would be more comfortable if we made the error a better message instead of a panic and allow for the user to decide if they are ok with skipping the check.
|
Thanks for the follow-up — I believe both of your asks are already addressed in the latest changes,
|
4 participants
Summary
Fixes two runtime panics in vendir's OpenPGP git-signature verification when running under
GODEBUG=fips140=only(Go native FIPS 140-3 strict enforcement):Changes
1. Key-fingerprint panic (
pkg/vendir/openpgparmor/armor.go)Parsing an OpenPGP key via
openpgp.ReadArmoredKeyRingcomputes an RFC 4880 v4 fingerprint using SHA-1 as part of key identification, not as a trust decision. This call is now wrapped incrypto/fips140.WithoutEnforcement, since the fingerprint is only used as a lookup key for later signature verification — it plays no part in verifying or trusting a signature, which continues to be enforced independently.2. Non-FIPS-approved signature algorithms (
pkg/vendir/fetch/git/verification.go)Before verifying a signature, vendir now inspects its declared hash and public-key algorithm. Under
GODEBUG=fips140=only:CheckArmoredDetachedSignatureis never called in this case, so nothing reaches the code path that would otherwise panic.verification.allowLegacySignatures: a new configuration field onDirectoryContentsGitVerification. When set totruefor a given git source, a non-approved algorithm is downgraded from an error to a logged warning, and verification proceeds — the actualCheckArmoredDetachedSignaturecall is wrapped incrypto/fips140.WithoutEnforcementonly in this opt-in case, since that's the only scenario where a non-approved algorithm reaches it.This addresses the production concern that outright failing on legacy-signed history (SHA-1/DSA-signed commits or tags) would break existing consumers with such history in their tree — they can now opt in deliberately via config, rather than vendir silently trusting or unconditionally rejecting legacy signatures for everyone.
allowLegacySignaturesis exposed as a configuration field rather than a CLI flag, so that systems which drive vendir purely through a YAML configuration document (rather than additional CLI arguments) are able to set it.Testing
pkg/vendir/fetch/git/verification_test.go).pkg/vendir/openpgparmor/armor_test.go).go build,go vet,go test) validated both with and withoutGODEBUG=fips140=only, confirming parity for non-FIPS builds and correct reject/warn behavior under strict enforcement.Backwards compatibility
No behavior change for any existing configuration:
Note on kapp-controller usage
kapp-controller (a primary downstream consumer of vendir) invokes
vendiras a subprocess, driving it entirely through a generated YAML configuration piped over stdin (vendir sync -f - --lock-file /dev/null) — it has no mechanism for passing additional CLI flags, which is whyallowLegacySignaturesis exposed as a configuration field here rather than a flag.We also confirmed that kapp-controller does not currently set
verificationat all when building the git configuration it passes to vendir. ItsAppFetchGitAPI type has no signature-verification concept today, so the code path this PR changes — including the previously-panicking behavior — is not reachable through anyApporPackageInstallresource in existing kapp-controller deployments. Wiring up verification support (and, by extension,allowLegacySignatures) in kapp-controller would be net-new work, tracked separately from this PR.